Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make warning about using performance tuning more explicit #2018

Merged
merged 1 commit into from
Jul 16, 2024
Merged

Make warning about using performance tuning more explicit #2018

merged 1 commit into from
Jul 16, 2024

Conversation

gdubicki
Copy link
Contributor

partially fixes #2017

@scylla-operator-bot scylla-operator-bot bot added do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 14, 2024
@scylla-operator-bot scylla-operator-bot
Copy link
Contributor

Hi @gdubicki. Thanks for your PR.

I'm waiting for a scylladb member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

tnozicka
tnozicka reviewed Jul 15, 2024
View reviewed changes
Copy link
Member

@tnozicka tnozicka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a general :::note to say that you should try performance tuning first on your staging cluster is ok, although that should be done for every change with any software when you care about your production availability. I'd avoid docs content based on issues that have just been opened / the colored wording

docs/source/performance.md Outdated

The only complete way to revert them to disable the tuning **and** restart all the nodes hosting your cluster.

There also were cases of performance degradation after applying the tuning (f.e. [scylladb/scylla-operator#2017](https://github.com/scylladb/scylla-operator/issues/2017)).
Copy link
Member

@tnozicka tnozicka Jul 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

referencing one open issue that has not yet been even triaged, doesn't fit a general claim made here (were cases)

@gdubicki
Copy link
Contributor Author

On second thought, I agree with you, @tnozicka. I was a bit emotional when I wrote this as I had spent most of the weekend fixing our cluster…

Can you just please reword the warning in this PR as you see fit?

@tnozicka
Copy link
Member

np, how about something like this:

:::{note}
We recommend that you first try out the performance tuning on a pre-production instance.
Given the nature of the underlying tuning script, undoing the changes requires rebooting the Kubernetes node(s).
:::

@tnozicka
Copy link
Member

/ok-to-test

@scylla-operator-bot scylla-operator-bot bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 15, 2024
@gdubicki
Copy link
Contributor Author

gdubicki commented Jul 15, 2024 •
edited
Loading

I changed the wording to yours, but I feel that as a plain note it will not work as intended. The warning type is required in my opinion to make it stand out enough. Wdyt, @tnozicka?

tnozicka
tnozicka approved these changes Jul 15, 2024
View reviewed changes
Copy link
Member

@tnozicka tnozicka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve
/lgtm
thanks

@scylla-operator-bot scylla-operator-bot bot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2024
@scylla-operator-bot scylla-operator-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gdubicki, tnozicka

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@scylla-operator-bot scylla-operator-bot bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2024
@tnozicka
Copy link
Member

/hold
please squash the commits

@scylla-operator-bot scylla-operator-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 15, 2024
@tnozicka tnozicka added the kind/documentation Categorizes issue or PR as related to documentation. label Jul 15, 2024
@scylla-operator-bot scylla-operator-bot bot removed the do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Jul 15, 2024
@tnozicka tnozicka added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Jul 15, 2024
@scylla-operator-bot scylla-operator-bot bot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jul 15, 2024
@tnozicka tnozicka removed lgtm Indicates that a PR is ready to be merged. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 15, 2024
@gdubicki gdubicki force-pushed the add-warning-for-performance-tuning branch from bc159ad to c95b49f Compare July 16, 2024 09:58
@scylla-operator-bot scylla-operator-bot bot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jul 16, 2024
@gdubicki
Copy link
Contributor Author

gdubicki commented Jul 16, 2024 •
edited
Loading

/hold please squash the commits

Sure, done! 😊

@tnozicka
Copy link
Member

/lgtm
thanks

@scylla-operator-bot scylla-operator-bot bot added the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2024
@scylla-operator-bot scylla-operator-bot bot merged commit 71aa4d5 into scylladb:master Jul 16, 2024
9 checks passed
@gdubicki gdubicki deleted the add-warning-for-performance-tuning branch July 16, 2024 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tnozicka tnozicka tnozicka approved these changes

@rzetelskik rzetelskik Awaiting requested review from rzetelskik

Assignees

@tnozicka tnozicka

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/documentation Categorizes issue or PR as related to documentation. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"Performance tuning" docs should be changed to prevent users from having performance-related incidents
2 participants
@gdubicki @tnozicka